-
Notifications
You must be signed in to change notification settings - Fork 59
Maven clean build failure when maven.build.cache.failFast=true #412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
created during hack.commit.push event |
| */ | ||
| private boolean isGoalClean(List<MojoExecution> mojoExecutions) { | ||
| if (mojoExecutions.stream().allMatch(mojoExecution -> "clean".equals(mojoExecution.getLifecyclePhase()))) { | ||
| LOGGER.warn("Build cache is disabled for 'clean' goal."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe info or debug level? Is a warning really needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elharo your comment makes total sense: info is perhaps better as this will give the user clear overview of what is going on with the cache during the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also prefer info instead of debug if warn is not okay.. IMHO, the user has to be informed that we skip the cache mechanism here because of the clean goal.
| * Cache configuration could demand to restore some files in the project | ||
| * directory (generated sources or even arbitrary content) | ||
| * If an error occurs during or after this kind of restoration AND a clean phase | ||
| * was required in the build : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: - ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point: I fixed the formatting of JavaDoc around that symbol, too.
| // cacheCandidate.getMojoDescriptor().getFullGoalName()))); | ||
| // org.apache.maven.api.plugin.Log.class, | ||
| // new DefaultLog(LoggerFactory.getLogger( | ||
| // cacheCandidate.getMojoDescriptor().getFullGoalName()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just delete the commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the commented code has a comment that it is related to mvn4. Therefore, we don't delete it. Is it okay, @sebtiem, if we delete the commented code? (git blame give me your name).
|
@elharo thank you for the review. I assume you follow the policy to clean up the code around specific changes related to a specific feature? It's a good practice, I wasn't aware that you guys follow it. Thanks for pointing out. |
|
@sparsick hey Sandra, I applied spotless (too late) - please re-run the pipeline when you have a chance. Thanks! |
olamy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
@sparsick The PR can't be associated to a milestone, because there are multiple open milestones. Please add the text "branch: master" to the description to the milestone where this PR belongs to. |
As discussed with @sebtiem we decided to skip the
build-cache-extensionexecution when the singlecleangoal is called.We wrote integration tests to verify that it is only skipped when the single
cleangoal is called.Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.